Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

332/Store slippage info on appData #629

Merged
merged 33 commits into from
Jul 1, 2022

Conversation

alfetopito
Copy link
Collaborator

@alfetopito alfetopito commented Jun 3, 2022

Summary

Part of #332

Edit 2022-07-01

No longer creating on appData per order.

Now stores the selected slippage, which will significantly reduce the amount of appData uploaded, while still capturing the original price.


Original description

Creating a new appDataHash for every order.

It's recalculated for every price quote.
It includes:

  • quote metadata (buy/sell amounts)

  • referrer metadata - if referral address is set and is valid

  • appCode - CowSwap or CowSwap-SafeApp

  • environemnt - local, dev, prod, barn, ens, etc

    To Test

  1. Load the page and open the dev console.
  2. Filter by appData
  3. On every price change you should see a debug log similar to this image

Screen Shot 2022-06-08 at 15 03 49

  • Make sure the fields match what you are seeing:
  • environment: pr
  • CID and appDataHash: calculated but won't exist yet on IPFS
  1. Place an order and open it on the browser https://barn.api.cow.fi/mainnet/api/v1/orders/<orderId>
  • Check the appDataHash in the order matches the last console debug log
  1. Filter the console log for UploadToIpfs
  • Check the appData related to your order has been uploaded, like the image

Screen Shot 2022-06-08 at 15 10 57

  1. Search for the appDataHash using https://codepen.io/pacara/pen/BadZgjg?editors=0101
  • Check it exists and the fields match the last console debug log

@alfetopito alfetopito self-assigned this Jun 3, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Jun 3, 2022

CLA Assistant Lite All Contributors have signed the CLA.

Copy link
Contributor

@anxolin anxolin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the direction of this PR is good.

So you are happy if we start using JOITAI for new state? it looks to me simple to read, write, and share logic. Also simpler to later move some pieces to libraries to be reused.

src/custom/components/AffiliateStatusCheck/index.tsx Outdated Show resolved Hide resolved
src/custom/constants/index.ts Show resolved Hide resolved
src/custom/hooks/useAppCode.ts Show resolved Hide resolved
src/custom/state/appData/atoms.ts Show resolved Hide resolved
src/custom/state/appData/atoms.ts Outdated Show resolved Hide resolved
src/custom/state/appData/types.tsx Outdated Show resolved Hide resolved
src/custom/state/appData/atoms.ts Outdated Show resolved Hide resolved
src/custom/utils/appData.ts Outdated Show resolved Hide resolved
@alfetopito alfetopito force-pushed the 332/generate-appDataHash-for-every-order branch from 0088bc9 to 55a1878 Compare June 8, 2022 13:46
@alfetopito alfetopito marked this pull request as ready for review June 8, 2022 13:53
@alfetopito alfetopito requested review from a team June 8, 2022 13:53
@github-actions
Copy link
Contributor

github-actions bot commented Jun 8, 2022

  • 🔭 GP Swap: CoW Protocol v2 Swap UI

@alfetopito alfetopito requested a review from anxolin June 8, 2022 15:42
@elena-zh
Copy link
Contributor

Hey @alfetopito , great job!

Some tiny issues/questions from my side:

  1. When I press on the Buy Cow button, I can see useAppDataHash in the console, and it shows an amount for a Sell token. However, there is no Sell token in the field. And that is weird
    buy cow

  2. When I swap a token pair, I can get up to 3 useAppDataHash logs in the console. See the video. Is this OK?

  3. There are no useAppDataHash logs when fees exceed from amount, not enough liquidity for a trade and unsupported token errors are displayed. Is this an expected behavior? (I feel that yes, but I'd better clarify)

Thanks!

@alfetopito
Copy link
Collaborator Author

Hey @alfetopito , great job!

Some tiny issues/questions from my side:

1. When I press on the Buy Cow button, I can see `useAppDataHash `in the console, and it shows an amount for a Sell token. However, there is no Sell token in the field. And that is weird
   ![buy cow](https://user-images.githubusercontent.com/70885163/173347055-61818fd4-5ed6-4235-83e3-049dc33fb780.jpg)

2. When I swap a token pair, I can get up to 3 `useAppDataHash` logs in the console. See the [video](https://watch.screencastify.com/v/KUczpADLPciNgRtoZzuq). Is this OK?

3. There are no  `useAppDataHash` logs when fees exceed from amount, not enough liquidity for a trade and unsupported token errors are displayed. Is this an expected behavior? (I feel that yes, but I'd better clarify)

Thanks!

There might be some unnecessary updates to the appDataHash, but the important part is that whatever is in the order matches what you see/sign. Is that the case?
Regardless, I'll check the points you raised.

@elena-zh
Copy link
Contributor

@alfetopito , the important part works great! I have just mentioned these edge cases to make sure they will not cause critical issues in future.

@alfetopito
Copy link
Collaborator Author

Checked the 3 points:

  1. When I press on the Buy Cow button, I can see useAppDataHash in the console, and it shows an amount for a Sell token. However, there is no Sell token in the field. And that is weird
    buy cow
  2. There are no useAppDataHash logs when fees exceed from amount, not enough liquidity for a trade and unsupported token errors are displayed. Is this an expected behavior? (I feel that yes, but I'd better clarify)

For this 2 cases, the values are being reset, but not logged. The log only happens when there's an actual change.
I tested it locally by adding the log for the values reset and it works as expected:

Screen.Recording.2022-06-20.at.16.49.08.mov
  1. When I swap a token pair, I can get up to 3 useAppDataHash logs in the console. See the video. Is this OK?

For this case, it's due to how the price quote is fetched. If you remove the console log filter you should see there is a fee quote update matching the appData update

When that differs (from what's already store in the appData), it'll be updated

Screen Shot 2022-06-20 at 16 46 58

@alfetopito alfetopito force-pushed the 332/generate-appDataHash-for-every-order branch from 3ba553f to b69dab7 Compare June 20, 2022 16:07
Copy link
Contributor

@anxolin anxolin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Greate PR. And happy with how you used jotai, I think doesn't change too much organization in terms of hooks/state/updaters\

WARNING! Still needs refactoring
WARNING! Needs to review if affiliate flow is still working as before
Most have been replaced by the correspondent sdk methods
* Updated stored types to contain lastAttempt rather than tryAfter

* New helper function to check when we can try to upload to ipfs again

* Improved logging for ipfs upload updater

* Changed soft upload failures log level from debug to warn

* Refactor: extracted helper function _actuallyUploadToIpfs

* Refactor: Renamed BASE_TIME_BETWEEN_ATTEMPTS to BASE_FOR_EXPONENTIAL_BACKOFF
* Updated state/affiliate

- Removed no longer needed state (appDataHash)
- Added new status referralAddress.isActive
- Updated associated actions, hooks and reducer
- Updated AffiliateStatusCheck to use new state

* Updated hooks/useAppData to use new state/affiliate state

* Updated useEffect deps to prevent unecessary re-renders

* Refactor: removed redundant variable

* Fixed issue where invalid referral would not be tagged as so
* Added `localWarning` for PINATA keys

* Displaying localWarning if any

* Added alternative warning display:

As a permanent toast notification

* Refactor: Renamed WarningPopupContent `message` to `warning`

* Added warning icon to toast notification

* Moved localWarning from Header to state/application

* Changed warning popup key to a more generic value

* Removed banner with warning in favor of the popup notification
@alfetopito alfetopito force-pushed the 332/generate-appDataHash-for-every-order branch from 42ecff3 to e625664 Compare June 29, 2022 11:04
* Ignore quoteId when checking if the order is unfillable

* Persiste quoteId from api to redux state

* Add quoteId to GpTrade class

* Pass quoteId down to appData

* Include quoteId on order placement

* Bumped cow-sdk to 0.0.15-RC.0
* Refactor: Replaced map upload queue with arrary

* Refactor: Using slice(0) to clone array instead of spread operator

* Refactor: using Array.some instead of Array.find

As I do not need to the stored element
* Bumop quote metadata version

* Added helper function to transfor Percent instances to bip string

* Refactored appData utils functions to use new quote metadata schema

Also changed the fn signature to accomodate different options if needed

* Updated useAppData to use slippage in the quote metadata

* Refactored useAppData interface

* Removed code that is not related to slippageBips for quote metadata

* Actually, _buildQuoteMetadata will never return undefined
* Increased upload to IPFS queue check interval to 1m

* Try to upload docs added to the upload queue right away
* Refactor: renamed useAddress to useAffiliateAddress

* Typo fix on comment

* Fixed issue with affiliate not valid displayed when there was no affiliate

* Refeset affiliate state whenever error is reset

* Referral address cannot be null, but undefined 🤦
@alfetopito alfetopito changed the title 332/generate app data hash for every order 332/Store slippage info on appData Jul 1, 2022
@alfetopito alfetopito merged commit 69242e3 into develop Jul 1, 2022
@alfetopito alfetopito deleted the 332/generate-appDataHash-for-every-order branch July 1, 2022 19:58
@github-actions github-actions bot locked and limited conversation to collaborators Jul 1, 2022
@elena-zh
Copy link
Contributor

elena-zh commented Jul 4, 2022

Just some updates to the PR description:

  • appdata log appears in the console when change a slippage value (not on every price update)
  • appdata log appears when share a referral link, and a user is connected to the Mainnet, and has never traded before
  • appdata and upload to IPFS will look like:
    image

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants